Shade the played span of the audio waveform as playback advances - #5826
Conversation
In the reading formats, the waveform now mirrors the native player's position: a clip-windowed copy of the bars tracks timeupdate from the mounted audio element, so the played span keeps the accent at full strength while the un-played remainder recedes. A track at rest keeps the waveform's usual weight, and a fitted cell — which mounts no player — is untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lukemelia
left a comment
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖]
Reviewed this as the next owner of AudioPreview, focused on the one thing that has to be exactly right for a progress overlay to mean anything — that the clip boundary lands on the actual playback position — plus the duration-source fallback, and whether the change stays out of the paths it shouldn't touch (fitted cells, headless prerender).
Bottom line: no blocking issues. The design is correct and tightly scoped, and the load-bearing invariant holds for a real reason rather than by luck.
What lands right, mechanistically:
- Clip boundary = playback position holds because the bars are uniformly time-spaced.
waveBarslays each bar atx = index * (100 / n)across the 0–100 viewBox, and the envelope is a uniform resample of the whole track, so x is linear in time.playedWidth = ratio * 100 = (currentTime / duration) * 100therefore names the x of the current instant, and auserSpaceOnUseclip rect fromx=0toplayedWidthcovers exactly the played span — partial last bar included. This is the invariant a future editor could silently break (e.g. switching to log-spaced or non-uniform bars); see the inline note on the clip rect. - Per-instance
clipIdis the right call.clipPathids are document-global and the reading formats can mount several previews on one page, so the moduleclipSerialgiving each instance its ownurl(#…)avoids cross-preview clip bleed. It's an instance field, so it's stable across re-renders. - Duration fallback + clamp is sound. Element duration wins once metadata arrives;
Infinity(live streams) andNaN(pre-metadata / emptied) both fall through to the extracted figure, and the[0,1]clamp absorbs any transient extract-vs-media mismatch. See the inline note for the one intended degradation. - The paths that shouldn't change don't. Fitted mounts no player and keeps the un-suffixed
.wave-svgmarkup, so its waveform is untouched; in a headless prerenderhasPlayedis always false, so the overlay/<defs>never render and there's no clip-id in the prerendered HTML. AndMidiPreviewdeliberately draws no player and no amplitude waveform, so there's no twin implementation this had to land on too.
Recommendation (non-blocking, follow-up): the new test pins only the extract-duration path at a fixed mid-track position. Consider also pinning the reset-to-rest transition (a seeking/emptied back to currentTime = 0 drops has-progress and removes the played layer) and the media-duration-wins branch — those are the two behaviors most likely to regress under a future refactor and neither is currently guarded. Detail in the inline thread on the test.
One clarification worth confirming rather than a change: the overlay is drawn after the base rects and painted at full fill-opacity over the 0.45 base, so the played region shows a single full-strength accent (no additive darkening from the dimmed base showing through). That's the intended read and it's correct as written.
| </g> | ||
| <defs> | ||
| <clipPath id={{this.clipId}}> | ||
| <rect x='0' y='0' width={{this.playedWidth}} height='100' /> |
There was a problem hiding this comment.
[Claude Code 🤖] Confirmation (non-blocking), and a guard-rail for the next editor.
This one clip rect is what makes the whole feature correct: width={{this.playedWidth}} = ratio * 100 marks the playback position only because the bars are uniformly time-spaced. waveBars places bar i at x = i * (100 / n) across the 0–100 viewBox, and the envelope is a uniform resample of the full track, so x is linear in playback time; with clipPathUnits defaulting to userSpaceOnUse, the 0 → playedWidth window covers exactly the played span, partial trailing bar included.
The fragility to flag: if the bar layout ever becomes non-uniform (log/mel spacing, silence-trimmed edges, a variable slot width), this mapping breaks silently — the highlight would drift from the true position with no test failing, because nothing here ties playedWidth back to the bar geometry. Nothing to change now; just the assumption to keep in mind whenever waveBars is touched.
There was a problem hiding this comment.
[Claude Code 🤖] Agreed, and the guard-rail is worth keeping visible. The new reset-to-rest and media-duration tests exercise playedWidth end-to-end but still don't tie it back to the bar geometry — they'd keep passing if waveBars moved to non-uniform spacing while the clip stayed linear. So this assumption remains the thing to re-check whenever waveBars changes; nothing to do on this PR.
| // The element's own duration wins once metadata arrives; before that (or | ||
| // in a context where the media never loads) the extracted figure stands in. | ||
| let duration = | ||
| Number.isFinite(el.duration) && el.duration > 0 | ||
| ? el.duration | ||
| : Number(this.args.model?.durationSeconds); | ||
| if (!Number.isFinite(duration) || duration <= 0) { | ||
| this.playedRatio = 0; | ||
| return; | ||
| } | ||
| this.playedRatio = Math.max(0, Math.min(1, el.currentTime / duration)); |
There was a problem hiding this comment.
[Claude Code 🤖] Confirmation (non-blocking).
The source ordering is right: el.duration wins once metadata loads, and both the pre-metadata NaN and the live-stream Infinity cases fail the Number.isFinite(...) && > 0 guard and fall through to the extracted durationSeconds. The final Math.max(0, Math.min(1, …)) absorbs the transient case where the extract duration disagrees slightly with the media's own before metadata arrives, so playedRatio can't escape [0,1].
The one intended degradation to be aware of: when neither source yields a positive finite duration, playedRatio is pinned to 0, so has-progress never applies and the overlay never renders — the track just plays with a static full-strength waveform. That's a reasonable fallback; noting it so it reads as deliberate rather than a missed case.
There was a problem hiding this comment.
[Claude Code 🤖] Confirmed. The new prefers the media element duration once metadata loads test now takes the el.duration-wins branch explicitly (finite duration of 20 over the extracted 10s), which the earlier test never did. The pinned-to-0 degradation when no source yields a positive finite duration is still deliberately unguarded — it's the intended static-waveform fallback, not a case worth a test.
| test('the audio waveform shades the played span as playback advances', async function (assert) { | ||
| let { AudioDef } = await loader.import<typeof AudioDefModule>( | ||
| `${baseRealm.url}audio-file-def`, | ||
| ); | ||
| let { WaveformMetadataField } = await loader.import< | ||
| typeof MetadataFieldsModule | ||
| >(`${baseRealm.url}file-formats/metadata-fields`); | ||
|
|
||
| let audio = new AudioDef({ | ||
| id: 'http://example.com/audio/take.wav', | ||
| url: 'http://example.com/audio/take.wav', | ||
| sourceUrl: 'http://example.com/audio/take.wav', | ||
| name: 'take.wav', | ||
| contentType: 'audio/wav', | ||
| contentSize: 2_646_078, | ||
| duration: 10, | ||
| waveform: new WaveformMetadataField({ | ||
| decodeStatus: 'ok', | ||
| barsJson: JSON.stringify(Array.from({ length: 32 }, () => 0.5)), | ||
| barCount: 32, | ||
| }), | ||
| }); | ||
|
|
||
| await renderCard(loader, audio, 'isolated'); | ||
| assert | ||
| .dom('[data-test-audio-preview] .wave-svg') | ||
| .exists('the waveform renders'); | ||
| assert | ||
| .dom('[data-test-audio-waveform-played]') | ||
| .doesNotExist('a track at rest marks nothing as played'); | ||
|
|
||
| let player = find('[data-test-audio-player]') as HTMLAudioElement; | ||
| // No media loads in this environment (the src 404s), so the element's own | ||
| // currentTime/duration never become usable. Shadow currentTime with an own | ||
| // property so the handler reads a definite position and falls back to the | ||
| // extracted duration, independent of media state. | ||
| Object.defineProperty(player, 'currentTime', { value: 5 }); | ||
| player.dispatchEvent(new Event('timeupdate')); | ||
| await settled(); | ||
|
|
||
| assert | ||
| .dom('[data-test-audio-waveform-played]') | ||
| .exists('playback marks the played span'); | ||
| assert | ||
| .dom('[data-test-audio-waveform-played]') | ||
| .hasAttribute( | ||
| 'data-test-audio-waveform-played', | ||
| '50', | ||
| '5s into a 10s track clips the played layer at half the waveform', | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] Test-coverage note (non-blocking, follow-up).
This pins the core well — the extract-duration fallback, the ratio math, and the clip window at a mid-track position all get exercised, and the at-rest negative case is asserted first. What it doesn't yet pin, and what's most likely to regress under a later refactor:
- Reset to rest. After the played layer exists, a
seeking/emptiedwithcurrentTimeback at 0 should drophas-progressand remove[data-test-audio-waveform-played]again. Nothing guards that the overlay tears down, so a change that leftplayedRatiosticky wouldn't fail here. - The media-
duration-wins branch. The test shadowscurrentTimeand leans on the extract duration because no media loads; theel.duration-preferred path (finite, > 0) is never taken. A one-liner definingdurationalongsidecurrentTimewould cover it.
Both are cheap add-ons in the same test style; fine as a follow-up rather than a blocker on this change.
There was a problem hiding this comment.
[Claude Code 🤖] Both branches are now pinned in ce37cdc2a4:
- Reset to rest —
the audio waveform tears down the played span when playback resets to rest: after atimeupdateestablishes the played layer, aseekingwithcurrentTimeback at 0 drivesplayedRatioto 0, and the test asserts[data-test-audio-waveform-played]no longer exists — so a sticky-ratio regression would fail here. - Media-
duration-wins —the audio waveform prefers the media element duration once metadata loads: shadows the element with a finitedurationof 20 alongsidecurrentTime5 and asserts the played layer clips at25, not the50the extracted 10s would give — so it can only pass via theel.duration-preferred branch.
Kept in the same style as the existing test (own-property shadowing since no media loads); lint clean. Local full-suite run is currently blocked by the base realm not booting on :4201, so CI is the end-to-end check.
Add two integration tests alongside the existing played-span test: a seek back to the start drops the overlay, and the media element's own duration takes precedence over the extracted figure once metadata loads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
[Claude Code 🤖] Addressed the review's one follow-up (the test-coverage recommendation) in |
backspace
left a comment
backspace
left a comment
There was a problem hiding this comment.
looks good
while exercising this I found that MP3s don’t have waveforms; I didn’t see an issue, Claude had a diagnosis
I could open an issue with its further details?
An MP3 envelope bar is the RMS across its granules, but the bars were scaled by the single loudest granule's amplitude. On the exponential 2^((global_gain - 210)/4) scale one dominant transient granule puts even its own bar near 1/sqrt(granules per bar) of that peak, so every bar collapsed toward zero and MP3 waveforms rendered as invisible slivers. Scaling by the loudest bar restores full-scale waveforms, matching how the WAV and decoded-PCM producers already behave (their raw RMS bars are never peak-scaled). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
[Claude Code 🤖] @backspace — the diagnosis in your screenshot checked out exactly against the code, and it's now fixed on this branch in 37ea8aa, so no separate issue is needed. What was wrong: The fix normalizes to the loudest bar instead (both the buffered and streaming extractors, via a shared helper), which matches how the WAV and decoded-PCM producers already behave — their raw RMS bars were never peak-scaled. There's a new regression test that plants a single transient frame among quiet ones and asserts the loudest bar still reaches full scale; the full One caveat: already-uploaded MP3s keep their persisted flat bars until their realm reindexes, since the waveform is extracted at indexing time. |
burieberry
left a comment
burieberry
left a comment
There was a problem hiding this comment.
Looks good. Just a couple findings from claude:
packages/base/file-formats/audio-preview.gts
- 98 [correctness] playedWidth quantizes to 0.1% of the track duration rather than of the rendered width, so on long media the clip window rounds to 0 while the dimming class is already applied.
- 84 [correctness] playedRatio is never reset on a model change, and the
emptiedsafety net only exists while the element is mounted.
packages/base/mp3-audio-def.gts
- 89 [stale-comment] This comment (and the parallel one at packages/base/file-formats/file-view-model.ts:197) still says the MP3 envelope is normalized to the track's own peak — the behavior this PR replaced; only the three comments inside mp3-meta-extractor.ts were updated.
…n comments hasPlayed now keys off the rounded playedWidth rather than the raw ratio, so the first moments of a very long track no longer dim the whole waveform while the zero-width clip window highlights nothing. Update the two sibling comments that still described the MP3 envelope as normalized to the track's own peak (the loudest granule) to say the loudest bar, matching the divisor the extractor now uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rm-reflects-playback-position-with # Conflicts: # packages/host/tests/integration/components/file-def-format-templates-test.gts
3 participants
In the reading formats (embedded/isolated), the default audio FileDef preview's waveform now reflects the native player's playback position, in the familiar audio-player idiom: the played portion renders in the full accent while the un-played remainder recedes to a dimmer shade, updating as the track plays and when the user seeks.
How it works
AudioPreviewmirrors playback as a 0–1 ratio from the mounted<audio>element'stimeupdate(plusseeking/emptied), preferring the element's own duration and falling back to the extracted duration before metadata arrives.clipPathwhose window width is the ratio, so partial-bar coverage is smooth rather than stepping bar-by-bar.Testing
file-def-format-templates-test.gts: at rest nothing is marked played; after atimeupdateat 5s of a 10s track, the played layer exists and clips at half the waveform.Integration | FileDef format templatesmodule passes (17 tests, 61 assertions).Resolves CS-12575.
🤖 Generated with Claude Code